Skip to content

gh-71810: fix corner case (length==0) for int.to_bytes() #138739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Sep 11, 2025
Merged

gh-71810: fix corner case (length==0) for int.to_bytes() #138739

merged 6 commits into from
Sep 11, 2025

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Sep 10, 2025 •
edited by bedevere-app bot
Loading 

>>> (0).to_bytes(0, 'big', signed=True)
b''
>>> (-1).to_bytes(0, 'big', signed=True)  # was b''
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    (-1).to_bytes(0, 'big', signed=True)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
OverflowError: int too big to convert

@skirpichev
pythongh-71810: fix corner case (length==0) for int.to_bytes()
33f60b9 
```pycon
>>> (0).to_bytes(0, 'big', signed=True)
b''
>>> (-1).to_bytes(0, 'big', signed=True)  # was b''
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    (-1).to_bytes(0, 'big', signed=True)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
OverflowError: int too big to convert
```
@bedevere-app bedevere-app bot mentioned this pull request Sep 10, 2025
Closed
@skirpichev skirpichev requested a review from vstinner September 10, 2025 10:56
@skirpichev skirpichev marked this pull request as ready for review September 10, 2025 11:22
vstinner
vstinner approved these changes Sep 10, 2025
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vstinner

This comment was marked as resolved.

Sign in to view
@vstinner
Copy link
Member

@serhiy-storchaka: I would suggest to backport this change to Python 3.13 and 3.14. What do you think?

@skirpichev skirpichev requested a review from vstinner September 10, 2025 11:55
serhiy-storchaka
serhiy-storchaka reviewed Sep 10, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there are no enough tests for signed=True. Please add also tests for (128).to_bytes(0, 'big', signed=True) and (-129).to_bytes(0, 'big', signed=True).

@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Sep 10, 2025
@skirpichev
Copy link
Contributor Author

Disclaimer:
This pr implements "the minimal change necessary to remove ambiguity" (c) @mdickinson

I was thinking about (0).to_bytes(0, 'big', signed=True) too, but that means rejecting also int.from_bytes(b"", ..., signed=True). That case looks for me as a special case, that aren't special enough to warrant compatibility break.

@serhiy-storchaka
Copy link
Member

Or better add several tests for extreme values and values just above the limit for all combination of signedness and n=0, 1, 2.

@skirpichev
Copy link
Contributor Author

It seems there are no enough tests for signed=True. Please add also tests for (128).to_bytes(0, 'big', signed=True) and (-129).to_bytes(0, 'big', signed=True).

Done.

There are such tests, but rather for "automatic" value for length parameter (determined by result):

cpython/Lib/test/test_long.py

Line 1262 in 1ce0553

def test_to_bytes(self):

Or better add several tests for extreme values and values just above the limit for all combination of signedness and n=0, 1, 2.

It seems to be a big refactoring of tests. Maybe it does make sense as a separate pr?

serhiy-storchaka
serhiy-storchaka reviewed Sep 10, 2025
View reviewed changes
serhiy-storchaka
serhiy-storchaka approved these changes Sep 11, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add much more tests. Anyway, LGTM. 👍

skirpichev and others added 2 commits September 11, 2025 10:04
@skirpichev @serhiy-storchaka
Apply suggestions from code review
aff76b8 
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@skirpichev
+ fix typo
60732b7
@vstinner vstinner merged commit 011179a into python:main Sep 11, 2025
45 checks passed
@miss-islington-app
Copy link

Thanks @skirpichev for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2025
@skirpichev @serhiy-storchaka @miss-islington
pythongh-71810: Fix corner case (length==0) for int.to_bytes() (pytho…
08fc362 
…nGH-138739)

```pycon
>>> (0).to_bytes(0, 'big', signed=True)
b''
>>> (-1).to_bytes(0, 'big', signed=True)  # was b''
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    (-1).to_bytes(0, 'big', signed=True)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
OverflowError: int too big to convert
```
(cherry picked from commit 011179a)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2025
@skirpichev @serhiy-storchaka @miss-islington
pythongh-71810: Fix corner case (length==0) for int.to_bytes() (pytho…
8681aea 
…nGH-138739)

```pycon
>>> (0).to_bytes(0, 'big', signed=True)
b''
>>> (-1).to_bytes(0, 'big', signed=True)  # was b''
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    (-1).to_bytes(0, 'big', signed=True)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
OverflowError: int too big to convert
```
(cherry picked from commit 011179a)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Sep 11, 2025

GH-138782 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Sep 11, 2025
@bedevere-app
Copy link

bedevere-app bot commented Sep 11, 2025

GH-138783 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Sep 11, 2025
@vstinner
Copy link
Member

Merged, thank you.

@skirpichev skirpichev deleted the int-bytes/71810 branch September 11, 2025 10:46
vstinner pushed a commit that referenced this pull request Sep 11, 2025
@miss-islington @skirpichev @serhiy-storchaka
[3.13] gh-71810: Fix corner case (length==0) for int.to_bytes() (GH-1…
7195d7f 
…38739) (#138783)

gh-71810: Fix corner case (length==0) for int.to_bytes() (GH-138739)

```pycon
>>> (0).to_bytes(0, 'big', signed=True)
b''
>>> (-1).to_bytes(0, 'big', signed=True)  # was b''
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    (-1).to_bytes(0, 'big', signed=True)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
OverflowError: int too big to convert
```
(cherry picked from commit 011179a)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
hugovk pushed a commit that referenced this pull request Sep 12, 2025
@miss-islington @skirpichev @serhiy-storchaka
[3.14] gh-71810: Fix corner case (length==0) for int.to_bytes() (GH-1…
440b83f 
…38739) (#138782)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
vstinner
vstinner reviewed Sep 13, 2025
View reviewed changes
Objects/longobject.c
@@ -1209,7 +1209,7 @@ _PyLong_AsByteArray(PyLongObject* v,
*p = (unsigned char)(accum & 0xff);
p += pincr;
}
else if (j == n && n > 0 && is_signed) {
else if (j == n && is_signed) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduced an undefined behavior: unsigned char msb = *(p - pincr); read is out of the bounds if n == 0. I wrote #138873 to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skirpichev @vstinner @serhiy-storchaka